Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding optional parameter to add a default a-tag class to the MvcLink… #21

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

royhyde-dd
Copy link

@royhyde-dd royhyde-dd commented Mar 14, 2019

… if the linkField does not have one defined. Fixing string comparisons so a-tag and alternateTags are created correctly.

… if the linkField does not have one defiend. Fixing string comparisons so a-tag and alternateTags are created correctly.
Copy link
Collaborator

@hollywoodinvegas hollywoodinvegas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the LinkField has a Class property already why do we need this extra property?
The property should be called linkClass not linkTagClass if it's actually needed to be consistent with the rest of the fields.
If we do need the property could we use the value in the "linkTagClass" and add it to the linkField before calling MvcLink?

@hollywoodinvegas
Copy link
Collaborator

@royhyde-dd please update the version in the csproj file to be 2.1.0.0 so that we can use this as a basis for a new nuget package

@royhyde-dd
Copy link
Author

If the LinkField has a Class property already why do we need this extra property?
The property should be called linkClass not linkTagClass if it's actually needed to be consistent with the rest of the fields.
If we do need the property could we use the value in the "linkTagClass" and add it to the linkField before calling MvcLink?

@hollywoodinvegas, in some cases, we want to add a class without the user needing to add a class. It is to resolve https://hub.deloittedigital.com.au/jira/browse/DSP-136. In that case, the class cm-image-block-link needs to be added to the a-tag for the link to render correctly.

Copy link
Collaborator

@hollywoodinvegas hollywoodinvegas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy for this to be merged, @royhyde-dd can you follow up on how we push this to nuget as version 2.1.0.0

@mt-dd
Copy link

mt-dd commented Mar 18, 2019

I think the behaviour here is a little weird as it'll use either the link's class or the supplied class. If a class is provided via linkTagClass but the content author already included a class, the linkTagClass value won't show up.

If both fields have a value should this create a HashSet<string> to split, merge, then re-join the two class inputs, and render that out as the final class string?

@hollywoodinvegas
Copy link
Collaborator

If both are set should we use both or does one override? In which case we should call the field defaultClass or overrideClass usually the FEDs are pretty good with stacking so you. Can probably just add both classes

…he XE, as there are issues with displaying components with these taks in 9.1 in the XE when the component only has a section tag surrounding it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants